Skip to content

test: validate Host and storage lifetimes on Node 22 - #5694

Open
seekskyworld wants to merge 3 commits into
apache:mainfrom
seekskyworld:fix/5640-node22-test-lifetimes
Open

seekskyworld wants to merge 3 commits into
apache:mainfrom
seekskyworld:fix/5640-node22-test-lifetimes

Conversation

@seekskyworld

@seekskyworld seekskyworld commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The dependency-authority startup test fails on Node 22 SQLite warnings because it treats stderr as failure. Wait for the child process's READY/exit protocol instead, retaining bounded stderr diagnostics on failure. CI reruns the Host/storage lifecycle fixtures on the minimum supported Node version after the Node 24 checks, and its policy test checks that version against the root engines lower bound.

Synced with main at 2463d86c3. Main now gives the in-memory peer recovery test a bounded settleWithin deadline, which owns its event-loop lifetime. The earlier file-wide keep-alive is removed; unrelated tests retain their normal cancellation behavior. Production heartbeat timers are unchanged.

Fixes #5640

Verification

  • Current head 0d17588bb: all 15 lifecycle tests pass on Node 22.19.0, without a file-wide keep-alive.
  • CI planner/workflow contracts: 87 passed. Temporarily bumping engines to 22.20.0 makes the minimum-version assertion fail, as intended.
  • Repository build, typecheck, lint, format, Desktop/UI knip, ASF headers and diff checks pass.
  • Before the original fix, Node 22.23.1 had 8 passes, 1 failure and 6 cancellations; the initial head passed these 15 tests on 22.19.0, 22.23.1 and 24.18.1. Its complete Node 22.19 Storage/Host suites passed 1,438/2,090 tests respectively. Those full suites were not rerun after this main synchronization.
  • Native Desktop E2E was not run locally. CI adds a focused minimum-Node check rather than a second full matrix.

Hosted check

The initial head passed run 35980890926. Checks for 0d17588bb are pending.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex reproduced the failures, implemented the test/CI changes, ran validation and drafted this description under human direction.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — test fixture lifecycle and CI validation change as described above
  • No

Generated-by: OpenAI Codex
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 24, 2026

@hqhq1025 hqhq1025 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.

I found no substantiated P0–P3 issue in this head. The change keeps the in-memory duplex test alive during Node 22 execution, makes the managed-dependency child startup protocol wait for READY or exit rather than treating stderr as immediate failure, and adds a Node 22.19.0 CI rerun after the workspace build. On the old base, the focused 15 tests gave 8 pass, 1 fail, and 6 cancelled under Node 22.22.1; on this head they passed 15/15 under Node 22.22.1 and Node 24.18.1. The current-head CI test succeeded and shows the Node 22.19.0 target tests passing.

This is test/CI-only: no runtime, engines, or schema migration change. I did not run the whole Node 22 matrix or cross-platform tests; the new CI step covers only these two lifecycle suites.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@me2seeks me2seeks 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.

Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.

Summary

The PR fixes two Node 22-only test-infrastructure failures and adds a focused Node 22.19.0 CI lane: (1) a keep-alive interval in resumable-peer-stream.test.ts so in-memory duplex transports outlive the test, and (2) a READY/exit-based startup protocol with bounded stderr diagnostics in the storage dependency-authority crash test, replacing "any stderr = failure". Both claimed problems are real and verified in base code; the fixes sit at the test-fixture layer with no production change, and the CI step correctly reuses the already-built dist/ output.

Findings

  1. [P3] packages/runtime-host/src/tests/resumable-peer-stream.test.ts:49-50, packages/storage/src/tests/managed-dependency-environment-crash.test.ts:189-190 — New explanatory comments are written entirely in Chinese. Across packages/*/src, CJK appears only in test string literals and zh-CN locale docs, never in explanatory comments; in an ASF project this hurts reviewability for the wider community. Translate to English.

No other material findings. Verified non-issues, adversarially checked: the Node-22 CI gate (ci.yml:528,534) can only be true when code=true (non-empty workspace closure ⇒ code=true in scripts/ci-test-plan.mjs:456-523; full plan sets code: true at :436), so npm ci and Build always precede it; dist/__tests__/*.test.js exist per the test:dist scripts; the native dep fs-native-extensions (imported via managed-dependency-environment.ts:50) ships per-platform N-API prebuilds, ABI-stable across the Node 24→22.19.0 mid-job switch; the new steps are the job's last, so the second setup-node affects nothing downstream; contains(standard_workspaces,'packages/storage') matches the existing packages/eval precedent (ci.yml:348) and no packages/storage* sibling workspace exists. The keep-alive's premise is confirmed by the deliberately unref'd production heartbeat (resumable-peer-stream.ts:117), and the SQLite warning premise by require('node:sqlite') at managed-dependency-environment.ts:341. Mutation check: reverting either fixture fix fails the new Node 22 CI step; the policy test (scripts/ci-workflow-policy.test.mjs:1177-1191) pins gate count, version, command, both paths, and absence of continue-on-error. CI rollup: label pass, test pass (26m19s) on a diff that selects both gated surfaces, so the new lane executed green.

Verdict

merge-ready — both failures are real, fixes are minimal and correctly layered, CI enforcement is sound; only comment language is worth a touch-up.

@orangeCatDeveloper orangeCatDeveloper 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.

Root causes check out and the fixes sit at the right layer. Three minor (P3) notes inline, none blocking.

Comment thread .github/workflows/ci.yml
Comment thread packages/runtime-host/src/__tests__/resumable-peer-stream.test.ts Outdated
Comment thread packages/runtime-host/src/__tests__/resumable-peer-stream.test.ts Outdated
Comment thread packages/storage/src/__tests__/managed-dependency-environment-crash.test.ts Outdated
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Signed-off-by: seekskyworld <djh1813553759@gmail.com>

@Astro-Han Astro-Han 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.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review from 9dd0d387 (previously reviewed, no findings) to 0d17588b. The update is a forward merge of main (2463d86c, no conflicts per --remerge-diff) plus one author commit, 0d17588b.

Author changes in 0d17588b:

  • resumable-peer-stream.test.ts: removes the setInterval keep-alive that was the core fix in the previously reviewed head. The PR no longer changes this file; it is now identical to main. Main's #5786 wrapped the one-way-blackhole recovery wait in settleWithin(), whose ref'd setTimeout keeps the event loop alive across that wait, so the keep-alive is redundant. I verified this: with a fresh build:test of this head, the CI step's two suites (resumable-peer-stream, managed-dependency-environment-crash) passed 15/15 on Node 22.19.0 in 5 consecutive runs.
  • ci-workflow-policy.test.mjs: the hard-coded 22.19.0 assertion is now derived from the lowest lower bound in root engines.node. On main that is ^22.19.0 || >=24.0.0, which parses to 22.19.0. Unsupported range syntax fails loudly. The policy test passes in CI (ok 87).
  • The storage test comment is translated to English. No behavior change.

Prior findings: none.

New findings: none at P0–P3. Note: after this commit, the PR title and description overstate the change. The runtime-host test is no longer modified, and the PR is now the CI floor step, the storage READY-protocol fix, and the policy test.

CI / mergeability: MERGEABLE/BLOCKED. Hosted test is red. The only failing step is Storybook smoke: settings-pages.stories.tsx:2835 (product-settings-pages--usage-long-tail / --usage-narrow) reports "expected 168.03125 to be less than or equal to 168". That is a known main-side sub-pixel flake in the Usage time-cell width check. This PR does not touch that story or the Settings Usage surface, so it is not PR-caused. Main's latest CI run skipped Storybook, so it cannot serve as a direct comparison. A rerun is needed for a green required gate. Because Storybook smoke runs earlier in the job, the PR's own "Test Node 22 Host and storage lifetimes" step was skipped on this head. The local Node 22.19.0 run above stands in for it until a rerun reaches that step.

Not covered: Windows/macOS on the Node floor; full Node 22 matrix.

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

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(runtime-host): tests fail on Node 22, which engines declares as supported but CI never runs

5 participants