Skip to content

Conversation

gabritto
Copy link
Member

@gabritto gabritto commented Aug 28, 2025

According to the Go language spec:

The return value of recover is nil when the goroutine is not panicking or recover was not called directly by a deferred function.

So the panic recovery in server.go wasn't working, because the deferred function wasn't directly calling recover() (instead, it would call s.recover() which would call recover(), which returned nil even if a panic happened).
I got rid of the indirection when calling recover(). A downside to it is that we now have to pass a request message to the handle... methods, because the recovery code accesses request message properties.

This PR also makes the fourslash test failure updater script throw when it detects an unrecovered panic, because that means go test will have stopped as soon as the panic happens, and since it didn't run all tests, we can't know which tests fail and which don't.

This should unblock #1553.

Copy link
Contributor

@Copilot 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.

Pull Request Overview

This PR fixes a critical panic recovery mechanism in the language server and improves test reliability. The main issue was that panic recovery wasn't working due to Go's requirement that recover() must be called directly by a deferred function.

  • Fixed panic recovery by removing indirection in recover() calls
  • Updated handler signatures to pass request messages for recovery context
  • Enhanced test script to detect and fail on unrecovered panics

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/lsp/server.go Fixed panic recovery by changing handler signatures to pass request messages directly and removing indirection in recover calls
internal/fourslash/_scripts/updateFailing.mts Added panic detection to prevent test result corruption when unrecovered panics occur

Copy link
Member

@iisaduan iisaduan left a comment

Choose a reason for hiding this comment

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

Looks good, I merged it to autoimports and it works. Thanks!

@iisaduan iisaduan added this pull request to the merge queue Aug 28, 2025
Merged via the queue into main with commit f89a64d Aug 28, 2025
22 checks passed
@iisaduan iisaduan deleted the gabritto/serverrecover branch August 28, 2025 22:51
@alexwork1611
Copy link

Thank you for your work!

zshannon pushed a commit to zshannon/typescript-go that referenced this pull request Oct 6, 2025
zshannon pushed a commit to zshannon/typescript-go that referenced this pull request Oct 6, 2025
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.

4 participants