-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: process exit before stdout/stderr are properly flushed #33213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix: process exit before stdout/stderr are properly flushed #33213
Conversation
|
|
not sure why semantic PR is not happy, i added the suggested line to line 4 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@Moumouls Do you have an example output / screenshot of what this looks like? I can't say that I've seen this exactly before. |
| } else { | ||
| setImmediate(resolve) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stream drain event may not fire causing unnecessary delay
Medium Severity
The waitForStreamDrain function waits for the 'drain' event when writableLength > 0. However, Node.js only emits the 'drain' event after a write() call returns false (indicating the buffer was full). If there's buffered data but all writes succeeded (returned true), the 'drain' event will never fire, causing the code to always wait for the 5-second safety timeout. This could add significant delays to CI builds. A more reliable approach would be to poll writableLength until it reaches 0, or use setImmediate to allow the event loop to flush pending writes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Moumouls Could you address whether this comment is relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not 100% sure about the correct approach here. The issue is very “low-level,” and I’m not an expert in that area. I was helped by Opus on this task, and the code seems logically sound. Does anyone on the Cypress team have expertise in this kind of stuff?
The 5-second timeout at the end of a full spec run doesn’t seem like an issue to me.
However, I’m not sure what the best approach is to ensure things happen ASAP and to verify that all logs are properly flushed.
If I were coding it myself, I’d probably go with a 1–3 second timeout. I believe that should be sufficient, since this feels more like a millisecond or nanosecond-level issue.
|
sure @jennifer-shehane
We tried a patch like this to capture the exit code, because we thought it was a GitHub Actions issue (and it still might be). That’s why, in my screenshot, you can see the coverage summary, but it’s cropped. yarn cypress:run --browser chrome --config-file cypress.config.ts; EXIT_CODE=$?; sleep 3; nyc report --reporter=text-summary; exit $EXIT_CODEI asked Opus 4.5, and it seems that nothing guarantees a process will wait for all standard streams to be fully flushed before exiting. In GitHub Actions, this means the exit code can be received and log streaming can be shut down before the full log has been transmitted. The issue seems also random sometimes we have the full report, so it sounds there is a race between the exit code and the STD. Example of a good working one ( with same scripts)
So my conclusion is: cypress seems to exit a little bit too fast, and it should wait a little bit to ensure in CI envs all the logs are correctly flushed |
|
@jennifer-shehane don't hesitate to tell me what i can do to help this PR to move forward ! The issue is quite annoying on our CI ahahha |
|
I have the same issue @jennifer-shehane, do you have any ETA for this one 🙏 ? |
|
Hi @jennifer-shehane, we’re still experiencing this issue. What can we do to help get this PR into the next release? |



Additional details
ON CI envs ( like github actions ), processs exit can terminate the process before stdout/stderr are properly flushed.
It lead to a cropped result summary table
Note
Prevents truncated terminal output in CI by guaranteeing buffered streams flush before exit.
waitForStreamDrain()and awaits it forprocess.stdoutandprocess.stderrinpackages/server/lib/cypress.tsexit()cli/CHANGELOG.mdwith15.8.3bugfix entry describing the CI output cut-off fixWritten by Cursor Bugbot for commit 3a128d6. This will update automatically on new commits. Configure here.
Steps to test
Create a lot of specs files, run Cypress on Ci env like Github Action, the result summary table is cropped.
How has the user experience changed?
No change
PR Tasks
Not sure how to add tests for this one.
cypress-documentation?type definitions?