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
Merged
doorgan merged 8 commits into from
Dec 14, 2025
Merged

fix: properly log when engine fails to initialize #244

doorgan merged 8 commits into from
Dec 14, 2025

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

mhanberg
mhanberg approved these changes Dec 14, 2025
View reviewed changes
Copy link
Member

@mhanberg mhanberg 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 to me.

@doorgan let me know if you have any comments, if not lets merge.

@doorgan
Copy link
Collaborator

doorgan commented Dec 14, 2025

Looks good to me too! I'll merge

@doorgan doorgan merged commit 81e1184 into elixir-lang:main Dec 14, 2025
36 checks passed
@github-actions github-actions bot mentioned this pull request Dec 14, 2025
Open
@badosu badosu deleted the badosu/initialize-error branch December 15, 2025 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhanberg mhanberg mhanberg approved these changes

@doorgan doorgan doorgan approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@badosu @doorgan @mhanberg