Fix lingering cli process after quitting#397
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cc8f81756
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9d4ec20a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
uraimo
force-pushed
the
fix-lingering-cli-process
branch
from
a9d4ec2 to
f6ac6a5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6ac6a5302
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
uraimo
force-pushed
the
fix-lingering-cli-process
branch
from
f6ac6a5 to
12ebc73
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12ebc7317a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
uraimo
force-pushed
the
fix-lingering-cli-process
branch
2 times, most recently
from
2d3b5f1 to
eb48fe7
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb48fe7587
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
uraimo
force-pushed
the
fix-lingering-cli-process
branch
from
eb48fe7 to
03b55fb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03b55fb32c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
uraimo
force-pushed
the
fix-lingering-cli-process
branch
from
03b55fb to
b86582c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b86582c3e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
uraimo
force-pushed
the
fix-lingering-cli-process
branch
from
b86582c to
1241958
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1241958582
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Done, nothing more to add here. |
|
Still working on this, the root cause is that the TTYCommandRunner deferred cleanup in run() is not actually being called when the app terminates, will send a better localized fix. |
uraimo
force-pushed
the
fix-lingering-cli-process
branch
from
1241958 to
e9b65d1
Compare
|
After a few test runs with some added instrumentation (print) I noticed that the root cause was the TTYCommandRunner started by CodexStatusProbe still waiting for the result of /status in its run() method and not being able to call the defer to do the cleanup. There are multiple sleeps and the replies of codex are a bit unreliable, it seems that sometimes the response doesn't even come back at all. And in addition to this there was a bug here, with codex being identified as isCodex=false and going through the non-codex if, since the binary variable contains the full path, binaryName should have been used instead. Regarding this final version of the fix, other than fixing isCodex, the process registry is not directly inside TTYCommandRunner and does not force other classes to register/unregister their processes explicitly. The registry is necessary since the TTY status is so unreliable that it's very likely that the application will have to quit while the probe is performing one of its long waits. New tests have also been added for this. I've noticed that the fallback that uses chatgpt.com has issues related to high cpu usage too (the usual single core at 100%), but it looks like that someone else is already working on that. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45c9a34f59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@ratulsarna Hi, can you take a look when you have time? |
1 participant
Hi, this fixes the CLI process (codex in my case) lingering around at 100% core after the application has been closed normally clicking Quit.
It builds a registry for all the started processes and manually kills them when the application terminates, a brute force approach but mine/clanker other attempts to quit cleanly failed, probably because the quitting is happening while processing is ongoing on the CLI and sending a /exit is basically useless.
Processes screenshot after a restart with the hung-up "codex" from the previous session:
Tested and now the process are reliably cleaned up. Tested with codex but added the same mechanism to claude since it could likely be affected by the same issue.
I could see any CONTRIBUTING.md so I've just opened a PR, hope it's ok.