Repository navigation
test: validate Host and storage lifetimes on Node 22 - #5694
seekskyworld wants to merge 3 commits into
Conversation
Generated-by: OpenAI Codex Signed-off-by: seekskyworld <djh1813553759@gmail.com>
hqhq1025
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
- [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
left a comment
There was a problem hiding this comment.
Root causes check out and the fixes sit at the right layer. Three minor (P3) notes inline, none blocking.
Signed-off-by: seekskyworld <djh1813553759@gmail.com> Generated-by: OpenAI Codex
Generated-by: OpenAI Codex Signed-off-by: seekskyworld <djh1813553759@gmail.com>
Astro-Han
left a comment
There was a problem hiding this comment.
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 thesetIntervalkeep-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 insettleWithin(), whose ref'dsetTimeoutkeeps the event loop alive across that wait, so the keep-alive is redundant. I verified this: with a freshbuild:testof 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-coded22.19.0assertion is now derived from the lowest lower bound in rootengines.node. On main that is^22.19.0 || >=24.0.0, which parses to22.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.
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 boundedsettleWithindeadline, 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
0d17588bb: all 15 lifecycle tests pass on Node 22.19.0, without a file-wide keep-alive.Hosted check
The initial head passed run 35980890926. Checks for
0d17588bbare pending.AI use
Tool(s) and scope: OpenAI Codex reproduced the failures, implemented the test/CI changes, ran validation and drafted this description under human direction.
Checklist
Does this PR entail a change in behavior?