Fix userEvents panic#5793
Open
stefanhaller wants to merge 4 commits into
Open
Conversation
Update and friends enqueued onto a fixed 256-slot channel with a non-blocking send that panicked when the channel was full. That guard was firing in real use: - Toggling a directory of several hundred files into a custom patch (reliably): the operation runs on a worker behind a waiting status, whose spinner enqueues a content-only render on every tick, and over the long operation these outrun the UI loop and overflow the buffer. - Editing the config in an editor that suspends lazygit: the editor subprocess runs on the UI thread, so the loop drains nothing for the whole editing session, and the full refresh fired on resume fans out across every scope at once — a burst of updates that overflows before the just-resumed loop catches up. - Any time the UI thread blocks for a long time, the periodic refreshes keep enqueuing and eventually overflow. The 256-slot buffer was chosen deliberately, with the panic as a "should never happen" guard, to preserve two properties: FIFO ordering of same-goroutine Update calls (an earlier goroutine-per-Update design reordered them), and no self-deadlock (a blocking send from the UI thread would block against the loop that drains it). But a fixed channel can only offer those by crashing on overflow. Replace it with an unbounded, order-preserving queue: a mutex-guarded slice plus a buffered(1) doorbell channel that wakes the main loop's select. Enqueuing appends and rings the doorbell; the loop drains the slice to empty on each wake. This keeps FIFO order and never blocks the caller, so there is no self-deadlock and no overflow to panic on — under a stall the queue just grows and then drains. This also removes an inconsistency: updateContentOnly did a plain blocking send while update panicked, so the two paths disagreed on what happened when the queue was full. Both now share the same enqueue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Now that the queue is unbounded, its depth is a useful signal for understanding how the event loop behaves under load — and we expect it to look very different across builds (e.g. master, which carries the bounce-state-updates-to-ui-thread work, versus the v0.63.0 release this fix ships in). Track the deepest the queue has ever been and log an Info line whenever that record is broken, so the numbers show up in the log for later reasoning. The mark is session-wide and doesn't reset when the queue drains. gocui has no logger of its own, so it exposes the new depth through a handler (matching the existing SetFocusHandler / SetOpenHyperlinkFunc pattern) that the gui registers to log via its own logger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
winPty.Close called ClosePseudoConsole before closing the pipes. ClosePseudoConsole blocks until the child's pending output has been drained from the out pipe, but a stopped task's scanner goroutine quits draining as soon as the task is stopped, and on Windows the child isn't killed either — so with a pager still producing output, Close hung forever. It hung while holding the global PtyMutex and while the task's onDone sync.Once was executing, so the task's cleanup never finished: the next NewTask call blocked on <-notifyStopped while holding waitingMutex, every later task for that view queued up behind it, and onResize blocked on PtyMutex — a full UI freeze. (Reported by a user via go-deadlock's 30s watchdog; a regression from the ConPTY support introduced for v0.63.0.) The same flush hang could strike the background waiter's closeHpc if the child exited right after its task was stopped, wedging the waiter while it holds p.mu. Close our ends of the pipes first, and only then close the pseudoconsole, without taking p.mu in between: a broken pipe makes conhost's flush fail immediately instead of waiting for a reader, which lets ClosePseudoConsole return promptly and also unblocks a waiter already stuck in that flush. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fix a deadlock when a Windows pty task is stopped mid-output winPty.Close could block indefinitely, and it is called while holding the global PtyMutex and while the task's onDone sync.Once is executing, so blocking there wedges the task's entire cleanup chain: the next NewTask call blocks on <-notifyStopped while holding waitingMutex, every later task for that view queues up behind it, and onResize blocks on PtyMutex — a full UI freeze. (Reported by a user via go-deadlock's 30s watchdog; a regression from the ConPTY support introduced for v0.63.0.) ClosePseudoConsole is what blocks; before Windows 11 24H2 it can do so in two ways. It flushes the client's pending output into the out pipe, but a stopped task's scanner goroutine has already quit draining, so with a client that's still producing output the flush never completes; this can also wedge the background waiter's closeHpc, which runs with the pipes deliberately left open. And it waits for the console host to exit, but closing only delivers CTRL_CLOSE_EVENT to the attached client without terminating it, so a client that keeps running (git still computing an expensive diff, a pager waiting for input) keeps the host alive arbitrarily long. Run the teardown on a background goroutine so Close returns immediately no matter which of these strikes, and within it close our pipe ends before the pseudoconsole, without taking p.mu: breaking the pipes fails a pending flush fast, which also unblocks a waiter already stuck in one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #5756 we changed the userEvents channel to a fixed 256-slot channel with a non-blocking send that panicked when the channel was full. It turns out that this panic can happen in real use:
The 256-slot buffer was chosen deliberately, with the panic as a "should never happen" guard, to preserve two properties: FIFO ordering of same-goroutine Update calls (an earlier goroutine-per-Update design reordered them), and no self-deadlock (a blocking send from the UI thread would block against the loop that drains it). But a fixed channel can only offer those by crashing on overflow.
Replace it with an unbounded, order-preserving queue: a mutex-guarded slice plus a buffered(1) doorbell channel that wakes the main loop's select. Enqueuing appends and rings the doorbell; the loop drains the slice to empty on each wake. This keeps FIFO order and never blocks the caller, so there is no self-deadlock and no overflow to panic on — under a stall the queue just grows and then drains.
Fixes #5772.