Repository navigation
Conversation
) ProgramHandler spawned go func(){ p.Wait(); a.remove(p) }() before wish runs the program, but Bubble Tea v0.25.0 initializes the finished channel in Run, not NewProgram — an early waiter receives from a nil channel forever, leaking the goroutine and the registry entry (plus a data race on concurrent initialization). Tie removal to session-context cancellation instead: s.Context() is valid at handler time and the session always outlives the program (wish runs p.Run() synchronously in the session handler), so session-end pruning covers every session with guaranteed startup ordering. Adds TestWatchSessionRemovesOnSessionEnd.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 53 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![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: 251d562bd1
ℹ️ 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".
| // channel forever (and race with Run's initialization). The session | ||
| // always outlives the program (wish runs p.Run() synchronously in | ||
| // the session handler), so session-end pruning covers every session. | ||
| go a.watchSession(s.Context(), p) |
There was a problem hiding this comment.
When a client keeps its SSH connection open across sessions (for example, with ControlPersist or multiple channels on one Go SSH client), exiting the chat never triggers this cleanup. In the pinned SSH dependency, Session.Context() returns the connection's context, and closing a session only closes its channel. Wish v1.2.0 cancels its own child context after p.Run(), not s.Context(). Consequently, every ended chat on that connection retains its program and watcher and continues receiving broadcast attempts until the entire connection closes. Tie removal to program/session completion instead, and cover a closed session whose underlying connection remains live.
Useful? React with 👍 / 👎.
Closes #4515.
Problem
ProgramHandlerspawnedgo func(){ p.Wait(); a.remove(p) }()before wish runs the program, but Bubble Tea v0.25.0 initializes thefinishedchannel inRun, notNewProgram— an early waiter receives from a nil channel forever, leaking the goroutine and the registry entry (plus a data race on concurrent init). Codex raised this as P2 on #4384 (r4443380736); the PR merged as-is.Verified empirically:
Wait()on a never-runNewProgramprogram stays blocked past a 300ms grace window (throwaway test, not committed).Fix
Tie removal to session-context cancellation (
watchSession):s.Context()is valid at handler time and the session always outlives the program (wish runsp.Run()synchronously in the session handler perwish@v1.2.0/bubbletea/tea.go), so session-end pruning covers every session with guaranteed startup ordering and zero contact withp.finished.Tests
TestWatchSessionRemovesOnSessionEnd: watcher started pre-Run deregisters on context cancel, stays registered while live.go test -race -count=1 ./...inpkgs/chat: 4/4 pass.go vet+gofmtclean.Task: queue task 6430.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.