✨ Handle Java provider shutdown logs

[eemcmullan]

resolves #871

Summary by CodeRabbit

• Improvements
  • Clarified and simplified logging for the Java language server and provider: distinct messages for normal vs. error terminations, adjusted log levels, and improved punctuation.
  • Reduced noisy logs by classifying termination scenarios as "safe" (e.g., context cancellation or SIGTERM/SIGKILL) and logging accordingly.
  • Refined shutdown handling to avoid unconditional cancellation during stop.

[coderabbitai]

Walkthrough

Updated process shutdown and logging in the Java external provider: adjusted language-server error messages and punctuation, replaced a verbose stop log with a simple info, and added an isSafeErr helper to classify safe terminations and change Stop() cancel/log behavior.

Changes

Cohort / File(s)	Change Summary
Provider init/error logging external-providers/java-external-provider/pkg/java_external_provider/provider.go	Refined Init goroutine logging: changed wording for non-nil process termination, simplified stop log, and added comma in context-cancel message.
Service client stop behavior external-providers/java-external-provider/pkg/java_external_provider/service_client.go	Added isSafeErr(err error) bool; Stop() now classifies errors as safe/unsafe to decide logging level and removed unconditional cancel; uses syscall inspection for exit status.

Sequence Diagram(s)

sequenceDiagram
  participant Provider as Java Provider
  participant Process as Language Server (cmd)
  participant StopFn as Stop()
  participant isSafe as isSafeErr
  participant Logger as Logger

  Provider->>Process: start
  Note right of Process: Runs until termination
  Process-->>StopFn: returns error (err)
  StopFn->>isSafe: isSafeErr(err)
  alt safe termination
    isSafe-->>StopFn: true
    StopFn->>Logger: info "java provider stopped"
    Note right of StopFn #C7F9CC: no forced cancel
  else unsafe termination
    isSafe-->>StopFn: false
    StopFn->>Logger: error "java provider stopped with error"
    StopFn->>Provider: (no change to cancel handling here)
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• 🐛 Wait for Java lang. server to finish before shutdown #875 — Modifies service_client.go Stop() shutdown logic and shutdown sequencing; closely related to Stop() logging/termination handling.

Suggested reviewers

• pranavgaikwad
• jmle
• dymurray

Poem

"I nibbled at logs by lantern's glow,
Tamed noisy errors so outputs flow,
A helper paw checks how processes part,
Quiet hops home — no needless alarmed heart.
🐇🌿"

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes Check	⚠️ Warning	The changes in provider.go adjust language-server error handling messages and context cancellation formatting, which fall outside the scope of the linked issue’s focus on Java provider shutdown logs.	Extract the provider.go modifications into a separate pull request or narrow this PR to only include the Java provider shutdown log changes specified by issue #871.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly describes the primary change by indicating that shutdown logs for the Java provider are now being handled, and it remains clear and focused without unnecessary details such as file lists or vague terms.
Linked Issues Check	✅ Passed	The implementation in service_client.go introduces an isSafeErr helper to classify safe termination errors and adjusts Stop() logging to emit an info-level “java provider stopped” message for context.Canceled or SIGTERM/SIGKILL exit scenarios, directly addressing the linked issue’s requirement to handle “stopping java provider” logs with error "{"Stderr":null}".

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

• Public repositories are always opted into early access features.
• You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[shawn-hurley]

I agree that the logs should be error, and I wonder if instead of if else blocks that do the same thing, if we could handle that more cleanly

[shawn-hurley]

what type of error is this and does this happen when the provider stops successfully? wondering if we can get the exit code and if it zero?

[eemcmullan]

It comes from here https://github.com/konveyor/analyzer-lsp/blob/main/external-providers/java-external-provider/pkg/java_external_provider/service_client.go#L261 and it does occur whenever the Java provider successfully completes. I can look into the exit code

[eemcmullan]

@shawn-hurley I was able to remove this line now that 90b71b4 is in. I no longer see this retuning.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)

270-284: Invoke the stored cancellation function in Stop()
The cancelFunc stored on javaServiceClient is never called, so the exec‐command context isn’t canceled and the Java process may hang. Add a call to p.cancelFunc() in Stop() (e.g. before p.cmd.Wait()) to ensure the process is terminated even if RPC shutdown stalls.

🧹 Nitpick comments (1)

external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)

325-339: Well-structured helper for classifying safe termination scenarios.

The implementation correctly identifies expected shutdown conditions:

• context.Canceled for cancelled operations
• SIGTERM/SIGKILL for external process termination

The use of errors.Is and errors.As follows Go best practices for error handling.

Consider adding a brief comment explaining the rationale for classifying these conditions as safe, e.g.:

+// isSafeErr returns true if the error represents an expected shutdown condition
+// (context cancellation or termination signals) rather than an actual failure.
 func isSafeErr(err error) bool {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23783e7 and 7ca33c6.

📒 Files selected for processing (2)

• external-providers/java-external-provider/pkg/java_external_provider/provider.go (1 hunks)
• external-providers/java-external-provider/pkg/java_external_provider/service_client.go (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• external-providers/java-external-provider/pkg/java_external_provider/provider.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: test
• GitHub Check: benchmark (windows-latest, windows)
• GitHub Check: benchmark (macos-latest, mac)
• GitHub Check: test (windows-latest)

🔇 Additional comments (1)

external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)

18-18: LGTM: Import required for signal inspection.

The syscall import is necessary for checking SIGTERM/SIGKILL in the isSafeErr helper function.

[aufi]

LGTM