-
Notifications
You must be signed in to change notification settings - Fork 156
run cleanupRelay only once in TerminalIO and StandardIO #153
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: main
Are you sure you want to change the base?
Conversation
@adityaramani I think the formatting issues have been fixed |
Im gonna pull in the changes locally and test it out as well |
thank you, I've been depending on the workflow tests and it's not fair to you that they're not passing |
No worries at all! |
Tried running the tests in a loop and it failed with this, and there werent any other logs from the guest
I'll try combining your change with what I was testing out with the |
dang, I really thought that was going to solve it. I'm going to look at this again when I get home, I got caught in a literal sandpit 💀 |
Thanks! I think your changes look good - and I tested it after adding in a couple of other fixes (see here https://github.com/adityaramani/containerization/tree/vminitd-crash) Although I do think there maybe some other issues in the process handling flow, this change makes it more stable. Will wait for the others to review |
@adityaramani do you want me to cherry pick your commit? it's only partially signed so I don't know |
Yeah go for it. Take that commit in. If the signature are a problem just apply the diff |
ok done, but it says unverified |
Yeah. Maybe try removing me as author? |
Signed-off-by: Aditya Ramani <a_ramani@apple.com>
@adityaramani if I could ask, what are the specs of the machine you're testing on? |
It's an M2 Pro with 64 gb of ram. Are you asking cause you don't see the issue on your end? I need to run the test on a loop and see it failing once in like 15 times |
I was just wondering, I think it has to be a race condition then. increasing system load ( |
for proc in processes { | ||
let pid = proc.pid | ||
self.log?.debug("checking for exit of managed process", metadata: ["pid": "\(pid)", "exits": "\(exited)"]) | ||
self.log?.debug("checking for exit of managed process", metadata: ["exits": "\(exited)", "processes": "\(processes.count)"]) |
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.
Great catch 😆
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.
This looks good to me (and sorry for delay), but I'll wait for either @adityaramani or @crosbymichael to give it a second thumbs up.
LGTM! I think Michael should take a look as well |
possibly fixes #124