Skip to content
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

fix(core): collect all logs from forked processes #27778

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

dgreif
Copy link
Contributor

@dgreif dgreif commented Sep 4, 2024

Current Behavior

We have a monorepo with ~450 packages and run tasks with 16x parallelism in CI. We've recently noticed that running tasks with static logging can lead to random tasks losing logs. Even if the task is a simple echo SOMETHING, logs get lost sporadically.

After digging into the task runner, I found that the forked task runner currently collects logs from stdout and stderr, then joins those when the process exit event is emitted. The problem appears to stem from the fact that the exit event is occasionally emitted before the stdout and stderr streams are closed, leading to lost logs.

Expected Behavior

All logs should be collected, even if they are emitted after the process exit event.

I've updated the task runner to wait for the stdout and stderr streams to end before collecting logs.

Note: I didn't see any tests specifically for this file, but it should still run all the new code if theres is a happy path case of forked tasks with static logging.

@dgreif dgreif requested a review from a team as a code owner September 4, 2024 20:17
@dgreif dgreif requested a review from Cammisuli September 4, 2024 20:17
Copy link

vercel bot commented Sep 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Sep 5, 2024 0:18am

For forked processes, wait until stdout and stderr have closed before capturing static log output.
This avoids a race condition where some logs could be lost from parallel tasks.
@dgreif dgreif force-pushed the wait-for-forked-streams branch from 8e8d04f to b9e33ad Compare September 5, 2024 12:15
@rluvaton
Copy link
Contributor

rluvaton commented Sep 8, 2024

This seems like fixing the same bug as my pr from 7 months ago

@dgreif
Copy link
Contributor Author

dgreif commented Sep 9, 2024

@rluvaton great catch! I'm surprised your PR hasn't gotten any traction yet 😞. Which approach would you lean toward for fixing the issue? I see the merit in your approach, but I'd be worried that some new issue could easily pop up which starts keeping the child process alive again. With the fix I have here, we are less dependent on the child process exiting with specific timing. WDYT?

@AgentEnder AgentEnder merged commit c3709b2 into nrwl:master Dec 16, 2024
5 checks passed
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants