-
Notifications
You must be signed in to change notification settings - Fork 7.2k
tui: move session logging off the main thread #8979
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
|
All contributors have signed the CLA ✍️ ✅ |
9dd2f7e to
72eb131
Compare
72eb131 to
ddfa35d
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
@jif-oai Could you take a look at this? |
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddfa35d4f8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks for working on this, and for writing up the motivation. I agree with the direction of moving TUI session logging off the UI thread. Background plus buffered writes is exactly the right shape of solution here. I am more concerned about this part though:
Those logs are not only for post-mortems, we use them actively to debug live Codex CLI sessions while they are running. Losing durability until exit would make a class of "Codex hung" or "Codex misbehaved" issues much harder to diagnose. One thing to flag on process. I am currently in the middle of another change to our TUI logging pipeline around consistent session id propagation, and there is some overlap with what this PR touches. Because of that I am probably going to hold off on reviewing or merging this until that work lands so we do not end up doing the same surgery twice in slightly different ways. If this change is fixing a concrete, user visible problem, for example the input lag mentioned in #8976, it would really help to have a bit more diagnosis in the issue itself that shows session logging on the UI thread is the root cause. Even something like timings, traces, or a small repro would let us validate that we are fixing the right thing and not just a plausible theory. Happy to circle back once the logging work is in, and I appreciate you digging into this. |
|
One more thing to add. This part of the TUI stack is still under documented and under tested for how critical it is. Session logging sits right on the boundary between the UI thread, IO, and shutdown, so small changes can easily introduce subtle correctness or performance issues. Even if we keep most of this approach, I think it would be good to use this PR to add at least some basic coverage or instrumentation around things like flush on exit and dropped events so we can change this code with more confidence going forward. |
Summary
tuiandtui2.Motivation
Fix input lag under high disk usage by eliminating synchronous disk writes on the UI thread.
Testing
cargo test -p codex-tuicargo test -p codex-tui2Issue