Skip to content

Conversation

@fubhy
Copy link
Contributor

@fubhy fubhy commented Jan 13, 2026

The LSP server could deadlock during shutdown when background tasks or handlers attempted to send messages (logs, diagnostics, responses) to the outgoingQueue after the writeLoop had exited. With no consumer draining the queue, these blocking channel sends would previously hang forever.

Copilot AI review requested due to automatic review settings January 13, 2026 19:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@jakebailey
Copy link
Member

Thanks for looking into this. We really shouldn't have code that actually uses Background() if it's long running or interacts like this; there should be a path to plumb a proper context, I would think.

@fubhy
Copy link
Contributor Author

fubhy commented Jan 14, 2026

There should be a path to plumb a proper context, I would think.

Along these lines ... ?

@fubhy fubhy force-pushed the logger-deadlock-test branch from 1732720 to 84988c2 Compare January 14, 2026 13:00
@fubhy fubhy changed the title Potential deadlock in case of lsp shutdown Fix potential deadlock in case of lsp shutdown Jan 14, 2026
@fubhy fubhy force-pushed the logger-deadlock-test branch from 84988c2 to 3794e61 Compare January 14, 2026 13:07
@fubhy fubhy force-pushed the logger-deadlock-test branch from dc68319 to c2630cc Compare January 14, 2026 21:52
@fubhy
Copy link
Contributor Author

fubhy commented Jan 15, 2026

The latest CI error is again caused by this: #2485

@jakebailey
Copy link
Member

I don't know how you're hitting these races in CI so often when I don't even remember seeing them anywhere until now 😅

@fubhy
Copy link
Contributor Author

fubhy commented Jan 15, 2026

I don't know how you're hitting these races in CI so often when I don't even remember seeing them anywhere until now 😅

Hah I guess that's just because I'm working on this in so many incremental steps because I'm so inexperienced with this codebase (and Go, frankly) that I trigger more CI runs than you maybe? ;-)

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is good, now.

@andrewbranch andrewbranch added this pull request to the merge queue Jan 16, 2026
Merged via the queue into microsoft:main with commit a2a8311 Jan 16, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants