Skip to content

fix: properly log when engine fails to initialize #244

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.

Sign up for GitHub

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

Jump to bottom
Open
badosu wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: properly log when engine fails to initialize #244

badosu wants to merge 8 commits into from

Conversation

@badosu
Copy link
Contributor

@badosu badosu commented Dec 5, 2025 •
edited
Loading

Previously whenever there was some error initializing the project node a few things would happen:

  • The node supervisor start code would fail with match error
  • The server would shutdown via init_timeout
  • There would be no indication as to the reason, except reading the node message debug message in expert.log

Now:

  • We properly respond to an initialize request with a server not initialized error
  • We gracefully shut down the node processes
  • We send an error message to the client with the last message sent by the node:
Node exited with status 127, last message: /home/badosu/.local/share/mise/installs/elixir/1.18.4-otp-26/bin/elixir: line 244: exec: erl: not found

This PR is motivated as an attempt to mitigate one of the many situations where expert fails silently, e.g.: #241

Remarks

  • We additionally remove code related to waiting for didChangeConfiguration since it was noop

@badosu badosu force-pushed the badosu/initialize-error branch from d6c6ff4 to c93445d Compare December 5, 2025 22:46
@badosu badosu changed the title feat: Properly respond to and log initialization errors feat: properly respond to and log initialization errors Dec 5, 2025
@badosu badosu force-pushed the badosu/initialize-error branch from c93445d to 3672c0b Compare December 5, 2025 22:49
badosu
badosu commented Dec 5, 2025
View reviewed changes
badosu
badosu commented Dec 5, 2025
View reviewed changes
badosu
badosu commented Dec 5, 2025
View reviewed changes
badosu
badosu commented Dec 5, 2025
View reviewed changes
@badosu badosu force-pushed the badosu/initialize-error branch from 3672c0b to 4e099f7 Compare December 5, 2025 23:04
@badosu
Allow the engine to start asynchronously
dc9c8f1 
It can take many seconds until the engine is available, hence editors
might time out the initialization.

We still at least send an error $window/logMessage with the
initialization error when it happens.
@badosu badosu force-pushed the badosu/initialize-error branch from ff0aed9 to dc9c8f1 Compare December 9, 2025 08:04
badosu
badosu commented Dec 9, 2025
View reviewed changes
badosu
badosu commented Dec 9, 2025
View reviewed changes
badosu
badosu commented Dec 9, 2025
View reviewed changes
badosu
badosu commented Dec 9, 2025
View reviewed changes
badosu
badosu commented Dec 9, 2025
View reviewed changes
mhanberg
mhanberg reviewed Dec 9, 2025
View reviewed changes
mhanberg
mhanberg reviewed Dec 9, 2025
View reviewed changes
apps/expert/lib/expert/engine_node.ex Outdated
end

def on_exit_status(%__MODULE__{} = state, exit_status) do
Logger.debug(
Copy link
Member

@mhanberg mhanberg Dec 9, 2025

Choose a reason for hiding this comment

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

instead, lets put an info log in the 0 clause of the case, saying the engine shutdown

then in the error clause, put an error log saying it shutdown unexpectedly

mhanberg
mhanberg reviewed Dec 9, 2025
View reviewed changes
mhanberg
mhanberg reviewed Dec 9, 2025
View reviewed changes
mhanberg
mhanberg reviewed Dec 9, 2025
View reviewed changes
@mhanberg mhanberg changed the title feat: properly respond to and log initialization errors fix: properly log when engine fails to initialize Dec 9, 2025
@badosu
Address review concerns
fe14474 
- Also show errors in the client via window/showMessage
- Improve logging cases
- Only register with nodemapper when node started ok
- For now do not introduce full expert test
@badosu badosu force-pushed the badosu/initialize-error branch 2 times, most recently from 3a79611 to 318d0d6 Compare December 9, 2025 19:37
@badosu
Reintroduce full server testing with different approach
e381804 
Now we start manually all application children instead of ensuring the
application has started
@badosu badosu force-pushed the badosu/initialize-error branch from 318d0d6 to e381804 Compare December 9, 2025 19:49
mhanberg
mhanberg reviewed Dec 9, 2025
View reviewed changes
mhanberg
mhanberg reviewed Dec 9, 2025
View reviewed changes
@badosu badosu force-pushed the badosu/initialize-error branch from 21a1b68 to d168107 Compare December 9, 2025 20:22
mhanberg
mhanberg reviewed Dec 10, 2025
View reviewed changes
apps/expert/lib/expert/engine_node.ex

_error_status ->
Logger.error(
"Engine shut down unexpectedly, node exited with status #{exit_status}). Last message: #{state.last_message}"
Copy link
Member

@mhanberg mhanberg Dec 10, 2025

Choose a reason for hiding this comment

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

Add the root_uri here as well and any other log about the engine

Copy link
Contributor Author

@badosu badosu Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I added the project as a prefix but I'm not sure that's desired in the current incarnation. Let me know if this message format looks good to you.

@badosu badosu force-pushed the badosu/initialize-error branch from 415c190 to 5ae550e Compare December 10, 2025 16:40
@badosu
Copy link
Contributor Author

badosu commented Dec 14, 2025

Let me know if there are any pending issues to be addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhanberg mhanberg mhanberg left review comments

@doorgan doorgan doorgan left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@badosu @mhanberg @doorgan