[rush-lib, rush-client-core, rush-daemon-transport] Fix the tests that fail in the publish pipeline and in CI - #6107
Open
Sean Larkin (TheLarkInn) wants to merge 3 commits into
Open
Sean Larkin (TheLarkInn) wants to merge 3 commits into
Sean Larkin (TheLarkInn) wants to merge 3 commits into
Conversation
…the publish pipeline The publish pipeline sets FORCE_COLOR=1, which GitHub CI does not, and runs on a busy agent. Every publish run since #6046 merged failed in "rush retest": - rush-daemon-transport: a test parsed a PID that console.log had colored. Write it with process.stdout.write. - rush-client-core: findLoggedFatalError kept the colored stack frames of Node's crash report in the error message. Ignore ANSI escape sequences, and run the crash report tests with FORCE_COLOR=0 and FORCE_COLOR=1. - rush-client-core: a test's "sleep 0" could exit before its shell became "sleep 600", and the shell reaped it. Use "sleep 1". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d3da49f-95f3-4291-9026-0b0f8e38675c
…rocess runs before reading /proc Just after the 'spawn' event, a child can still be in execve, before /proc shows its environment or command line. libuv reports a spawn when the child's close-on-exec signal pipe reaches EOF. If the parent closes its own end of that pipe after the child's exec has closed the child's end, the parent's close is the last one, so EOF comes at once while the child is still in exec. A loaded CI runner can delay the parent that long: OperationGroupMarker.test.ts failed on Node.js v24 (ubuntu) in https://github.com/microsoft/rushstack/actions/runs/37041623922/job/110952865222. - OperationGroupMarker and NativeLockHolder: the child prints once it runs, and the test waits for that. "names only the PID of a holder whose command it cannot shorten" now reads a real command line, instead of passing on an empty one. - OperationGroupSpawnMarkReap and OperationMarkerInheritance: sleep cannot print, so the test polls until /proc shows the marker. Without the wait, "never signals a process with the marker that started over 1 s after the spawn mark" could pass only because the environment was not readable yet. - Move the /proc/uptime reader to UptimeTicksFixture.ts, to keep OperationGroupSpawnMarkReap.test.ts within the package's 100-line limit. An LD_PRELOAD shim that delays the parent's close forces the race. On Node.js 22 and 24, reading right after 'spawn' missed the environment in 151 of 180 runs with a node child and 145 of 180 with sleep; the new waits missed it in none. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d3da49f-95f3-4291-9026-0b0f8e38675c
…t timeout "kills tar, and deletes its partial archive and the sealed files, when aborted" ran under Jest's default timeout of 5 s. On Windows, the test's stand-in for the clone copies all 2 GB of its sparse file, and the test then waits up to 10 s for the partial archive and 5 s for tar to exit. It timed out on Node.js v26 (windows) in this PR's CI: https://github.com/microsoft/rushstack/actions/runs/37052175171/job/110987993537 In the previous run, where it passed, the file's tests took 5.0 and 5.3 s on Windows and 1.8 s on Linux. The test now has a timeout of 30 s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d3da49f-95f3-4291-9026-0b0f8e38675c
This branch has not been deployed
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
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
Every publish run since #6046 merged (1e18b16) has failed in "Rush retest", so nothing has been published since. The publish pipelines set
FORCE_COLOR: 1, which GitHub CI does not, and they run on busy agents. Under those conditions, three tests fail:B is a product bug: with
FORCE_COLORset, the error that the client reports for a crashed daemon included the stack of the daemon's crash report. A and C are test bugs.This PR's own CI then found D, a test that reads
/procof a process before the process has finished starting. Three other test files had the same bug; the second commit fixes all four. It also found E, a rush-lib test that can take longer on Windows than Jest's default timeout allows; the third commit gives it 30 s.Details
A. rush-daemon-transport
DaemonOperationGroupRecorder.test.ts, "keeps a detached child record until inherited streams close":With
FORCE_COLOR,console.log(child.pid)in the test's child script writes\x1b[33m<pid>\x1b[39m, andNumber()of that isNaN. The child now writes the PID withprocess.stdout.write(String(child.pid)).B. rush-client-core
DaemonDisconnect.test.ts, "findLoggedFatalError › returns the message of an unhandled rejection":With
FORCE_COLOR, Node.js colors its crash report although stderr is the launcher log, not a terminal. It wraps internal stack frames in\x1b[90m…\x1b[39m, soSTACK_FRAMEdid not match them and they joined the message. The other crash report tests passed only because their first frame is user code, which Node.js does not color; every frame of anERR_UNHANDLED_REJECTIONis internal. I checked Node.js 20.18.3, 22.23.3 and 24.12.0: each colors the report withFORCE_COLOR=1and none does without it.findLoggedFatalErrornow removes ANSI escape sequences before it reads the lines. The pattern isAnsiEscape's CSI pattern from @rushstack/terminal, which rush-client-core does not depend on. The crash report tests now run withFORCE_COLOR=0andFORCE_COLOR=1, so GitHub CI covers the colored report too. I considered starting the daemon withNODE_DISABLE_COLORSinstead, but that changes the environment the daemon runs in, and reading the log without escapes is enough.C. rush-client-core
connectOrStartDaemonLiveOwner.test.ts, "a live daemon owner that does not answer › names the parent that has not reaped an owner that exited", in 12802 only:The test runs
sh -c 'sleep 0 & echo $!; exec sleep 600'to get a zombie that its parent never reaps. Ifsleep 0exits before the shell runsexec, the shell reaps it (dash and bash reap finished background jobs after a builtin), so the zombie is gone before the test reads it. I probed this with the shell pinned to a CPU that 3 busy loops share, 300 runs each:sleep 0sleep 1The test now uses
sleep 1, asZOMBIE_PARENT_SCRIPTin rush-daemon-transport'sDaemonProcessStat.test.tsalready does. This is a margin, not a guarantee: the shell must reachexecwithin 1 s. The original test did not fail on my machine (0 of 23 runs with its shell pinned to a busy CPU), so the evidence for C is the probe and the 12802 log.D. Found by this PR's CI, not by the publish runs: rush-daemon-transport
OperationGroupMarker.test.ts, "finds only an exact entry of the environment that a process started with", on Node.js v24 (ubuntu) in job 110952865222:The test read the child's
/proc/<pid>/environright after the'spawn'event, which does not mean that the child's exec has finished. libuv reports a spawn when the child's close-on-exec signal pipe reaches EOF. Usually the parent closes its end of the pipe first, so the child's exec makes the last close, and EOF comes after the exec. If the parent is delayed betweenforkand its close, as on a loaded CI runner, the child's exec closes its end first, and the parent's close is the last one: EOF comes at once, while the child is still inexecveand/procshows no environment yet. #6046's CI hit the same race in run 36754086279, where a just-spawnedsleephad no command line yet; that test already waits for the process to sleep.To prove it, an
LD_PRELOADshim delays the parent's close of that pipe, and a large environment makes the child's exec slow enough to land in. Runs that found no marker in the child's environment, of 90 per cell:'spawn'(old)sleep, right after'spawn'(old)sleep, polled until/procshows the marker (new)To reproduce the race
delayclose.c, preloaded into the parent only, delays libuv's close of the signal pipe:probe.jsreads the environment right after'spawn', as the old test did:The delay must land inside the child's exec, which depends on the machine; on mine, 2100 to 2300 µs did.
Four test files read
/procof a process they had just spawned. They now wait until it runs:OperationGroupMarker.test.tsand rush-client-coreNativeLockHolder.test.ts: the child prints once it runs, and the test waits for that. InNativeLockHolder.test.ts, "skips a lock file whose PID now belongs to a process that started after it was written" reads the holder's command line, and "names only the PID of a holder whose command it cannot shorten" could pass on an empty one.OperationGroupSpawnMarkReap.test.tsandOperationMarkerInheritance.test.ts:sleepcannot print, so the test polls until/procshows the marker. Without the wait, "never signals a process with the marker that started over 1 s after the spawn mark" could pass only because the environment was not readable yet./proc/uptimereader ofOperationGroupSpawnMarkReap.test.tsmoves toUptimeTicksFixture.ts, to keep the file within the package's 100-line limit.E. Also found by this PR's CI: rush-lib
OperationBuildCacheDeferredWrites.test.ts, "kills tar, and deletes its partial archive and the sealed files, when aborted", on Node.js v26 (windows) in job 110987993537:The test ran under Jest's default timeout of 5 s. On Windows, its stand-in for the clone copies all 2 GB of the sparse file that tar archives, and then the test waits up to 10 s for the partial archive and 5 s for tar to exit. In the previous run, where it passed, the file's 6 tests took 5.0 and 5.3 s on Windows and 1.8 s on Linux; in this run they took 8.8 s. The test now has a timeout of 30 s.
GitHub CI does not set
FORCE_COLOR, which is why #6046's checks passed. The newFORCE_COLOR=1tests cover B in CI. SettingFORCE_COLORin.github/workflows/ci.ymlwould catch tests like A in every project; this PR does not change CI.After this merges, the publish pipelines need to run again. #6046's change files are still pending.
How it was tested
rush retestof both projects under the publish agent's conditions, Node.js 24.12.0 withFORCE_COLOR=1:FORCE_COLOR=1variants.FORCE_COLOR: 237 and 381 passed, 0 failed.FORCE_COLOR=1: all passed.DaemonDisconnect.ts, the newFORCE_COLOR=1test for B fails with the publish run's message, withoutFORCE_COLORin the environment.FORCE_COLOR=1.rush retestof both projects passed again with the same totals, on Node.js 24.12.0 withFORCE_COLOR=1, on Node.js 22.23.3, and on Node.js 20.18.3.rush retestof rush-lib on Node.js 22.23.3 (Linux): 139 suites, 1,884 passed, 0 failed. This PR's Windows CI jobs run the changed test on Windows.rush buildof the three projects: no warnings.